Tell workers to kill operations the scheduler no longer has executing on them - #2693
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
/build-image nativelink-worker-init |
|
/build-image nativelink |
|
Image built and pushed! |
|
Image built and pushed! |
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
There was a problem hiding this comment.
I have asked a clanker to review this one.
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
There was a problem hiding this comment.
Read through the whole change and traced it against the source — the mechanism is sound and the test coverage is genuinely strong (the is_executing_on_worker table-test across queued/assigned/requeued/reassigned/finished/unknown, plus the undeliverable-kill → evict test). A few notes inline on lock scope and Redis load.
One design-level point that doesn't map to a single line: this is a global, unconfigurable behaviour change. Previously an orphaned-but-running action ran to completion and warmed the action cache, so a later identical request hit cache; now it's killed, discarding in-flight work and the would-be cache entry. For long / expensive actions whose client merely dropped and will retry, that converts a cache hit into a full re-execution. Worth gating behind a config knob so operators with expensive long-running actions can opt out — the default is a policy call, but there should be an escape hatch.
(Not a blocker — CI is green and the core logic looks correct. Inline comments are suggestions.)
| return Ok(()); | ||
| } | ||
|
|
||
| let mut inner = self.inner.lock().await; |
There was a problem hiding this comment.
Lock held across store I/O. Phase 3 re-checks is_executing_on_worker (L1012) while holding self.inner — unlike phase 1, which correctly snapshots under the lock and does the store calls lock-free. This sweep fires during timeout/disconnect storms (many revoked ops at once), so the global scheduler mutex ends up held across N sequential state-manager round-trips (a Redis GET each on store-backed deployments), serializing all worker updates and matching behind network I/O.
Suggest keeping the phase-3 recheck lock-free (mirroring phase 2), collecting the confirmed set, then taking the lock only for the worker_notify_kill_operation sends. worker_notify_kill_operation already re-guards with contains_key + is_kill_requested, so the TOCTOU window stays tight without holding the lock across the store call.
| worker | ||
| .running_action_infos | ||
| .iter() | ||
| .filter(|(_, pending_action_info)| !pending_action_info.kill_requested) |
There was a problem hiding this comment.
Slot-leak corner for a live-but-unresponsive worker. Once kill_requested is set, the op is filtered out of every future sweep here, and nothing else re-sends or times out the kill itself. A dead worker is recovered by remove_timedout_workers, but a live worker that drops or ignores the KillOperationRequest holds its slot indefinitely while update_action silently swallows its updates (L377). Worth a comment noting that keepalive-timeout eviction is the sole recovery path here, or a bounded re-send.
| self.id | ||
| ) | ||
| })?; | ||
| pending_action_info.kill_requested = true; |
There was a problem hiding this comment.
Nit: kill_requested is set before send_msg_to_worker can fail just below. It's safe in practice because the caller (worker_notify_kill_operation) evicts + requeues on send failure, discarding this entry — but "flag set, message not delivered" reads like a latent slot leak. A one-line note that the flag's lifetime is bounded by eviction-on-send-failure would help the next reader.
|
/build-image nativelink-worker-init |
|
/build-image nativelink |
|
Image built and pushed! |
|
Image built and pushed! |
8889c0f to
d7394cd
Compare
2 participants
What and why
The scheduler never sent KillOperationRequest (only tests constructed one), so when an operation was finished server-side (client timeout, cancellation, execution deadline, requeue) the worker kept running it and holding its slot until the process ended on its own. This adds a periodic pass that asks the state manager whether each running operation is still executing on its worker and tells the worker to kill it if not. The worker's later report for a killed operation is not forwarded, so the kill's error cannot burn a retry on a re-queued copy.
How was this verified?
Unit tests only. is_executing_on_worker is table-tested across queued, assigned, requeued, reassigned, completed and unknown. A scheduler-level test drives a real client timeout while a single-slot worker holds the operation and checks that exactly one kill is sent, the worker's report returns Ok, and the slot frees for the next action; without the change, no kill is sent and the report fails with "already completed". A third test drops the worker channel and checks the worker is evicted when the kill cannot be delivered. Not yet run against a real deployment.
Risk
Behaviour change: an operation whose client vanished is now killed instead of running to completion and warming the AC. Every 5s each running operation costs one state-manager lookup, which is one Redis GET on store-backed deployments. New trait method on WorkerStateManager and WorkerScheduler. Wire format unchanged, the message already existed.
This change is